Introduce symbol versioning for clang-cpp #116556
Merged
+10
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The situation that required symbol versions on the LLVM shared library can also happen for clang-cpp, although it is less common: different tools require different versions of the library, and through transitive dependencies a process ends up with multiple copies of clang-cpp. This causes havoc with ELF, because calls meant to go one version of the library end up with another. I've also considered introducing a symbol version globally, but for example the clang (C) library and other targets outside of LLVM/Clang, e.g. libc++, would not want that. So it's probably best if we keep it to those libraries.
Member
|
@llvm/pr-subscribers-clang Author: Aaron Puchert (aaronpuchert) ChangesThe situation that required symbol versions on the LLVM shared library can also happen for clang-cpp, although it is less common: different tools require different versions of the library, and through transitive dependencies a process ends up with multiple copies of clang-cpp. This causes havoc with ELF, because calls meant to go one version of the library end up with another. I've also considered introducing a symbol version globally, but for example the clang (C) library and other targets outside of LLVM/Clang, e.g. libc++, would not want that. So it's probably best if we keep it to those libraries. Full diff: https://github.com/llvm/llvm-project/pull/116556.diff 3 Files Affected:
diff --git a/clang/tools/clang-shlib/CMakeLists.txt b/clang/tools/clang-shlib/CMakeLists.txt
index 298d3a9d18fec8..830f2b138ffa16 100644
--- a/clang/tools/clang-shlib/CMakeLists.txt
+++ b/clang/tools/clang-shlib/CMakeLists.txt
@@ -48,6 +48,14 @@ add_clang_library(clang-cpp
${_OBJECTS}
LINK_LIBS
${_DEPS})
+
+configure_file(simple_version_script.map.in simple_version_script.map)
+
+if (NOT LLVM_LINKER_IS_SOLARISLD AND NOT MINGW)
+ # Solaris ld does not accept global: *; so there is no way to version *all* global symbols
+ target_link_options(clang-cpp PRIVATE LINKER:--version-script,${CMAKE_CURRENT_BINARY_DIR}/simple_version_script.map)
+endif()
+
# Optimize function calls for default visibility definitions to avoid PLT and
# reduce dynamic relocations.
if (NOT APPLE AND NOT MINGW AND NOT LLVM_LINKER_IS_SOLARISLD_ILLUMOS)
diff --git a/clang/tools/clang-shlib/simple_version_script.map.in b/clang/tools/clang-shlib/simple_version_script.map.in
new file mode 100644
index 00000000000000..cb2306d1f59682
--- /dev/null
+++ b/clang/tools/clang-shlib/simple_version_script.map.in
@@ -0,0 +1 @@
+@LLVM_SHLIB_SYMBOL_VERSION@ { global: *; };
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index cde4a999ea2e74..304197667dad6e 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -27,7 +27,7 @@ if (NOT PACKAGE_VERSION)
endif()
if(NOT DEFINED LLVM_SHLIB_SYMBOL_VERSION)
- # "Symbol version prefix for libLLVM.so"
+ # "Symbol version prefix for libLLVM.so and libclang-cpp.so"
set(LLVM_SHLIB_SYMBOL_VERSION "LLVM_${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}")
endif()
|
Contributor
|
There's an existing PR for this here: #110758 |
Member
Author
tstellar
approved these changes
Nov 18, 2024
Collaborator
tstellar
left a comment
tstellar
left a comment
There was a problem hiding this comment.
This LGTM. I'm fine taking this version instead of my patch.
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/9808 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/4994 Here is the relevant piece of the build log for the reference |
Contributor
|
Hi this patch has broken the greendragon llvm bot. Link to failed build: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/15566/ |
rastogishubham
added a commit
that referenced
this pull request
Nov 20, 2024
This reverts commit 944478d. Reverted because of following error on greendragon ld: unknown options: --version-script clang: error: linker command failed with exit code 1 (use -v to see invocation)
Contributor
|
I have reverted the patch with 673b5dd to ensure the bot is green |
Member
Author
|
@tstellar, seems like we both overlooked the |
Member
Author
|
Same for if (NOT CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND
NOT MSVC AND NOT LLVM_LINKER_IS_SOLARISLD AND NOT MINGW) |
Collaborator
|
@aaronpuchert That looks fine to me. |
aaronpuchert
added a commit
that referenced
this pull request
Nov 20, 2024
The situation that required symbol versions on the LLVM shared library can also happen for clang-cpp, although it is less common: different tools require different versions of the library, and through transitive dependencies a process ends up with multiple copies of clang-cpp. This causes havoc with ELF, because calls meant to go one version of the library end up with another. I've also considered introducing a symbol version globally, but for example the clang (C) library and other targets outside of LLVM/Clang, e.g. libc++, would not want that. So it's probably best if we keep it to those libraries.
Member
Author
|
Went with a version that resembles the following condition and is a bit shorter. |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/1642 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/1496 Here is the relevant piece of the build log for the reference |
Member
Author
Seems the linker doesn't support the flag, but the CMake configuration log says: "Linker detection: unknown". So how do I detect this case? By detecting AIX? Maybe @llvm/pr-subscribers-backend-powerpc can help? |
Member
Author
|
Maybe We could also go with |
aaronpuchert
added a commit
that referenced
this pull request
Nov 21, 2024
There was a build bot failure on AIX after #116556, and who knows what other systems don't support symbol versioning. So let's limit this to Linux for now. We can always add more cases later.
Member
Author
|
We don't run into the same issue with the |
DanielCChen
added a commit
that referenced
this pull request
Nov 22, 2024
…script. (#117342) AIX BuildBot failed due to #116556 as AIX linker does not support version script. This PR is to fix the failure This PR is on behalf of [email protected]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The situation that required symbol versions on the LLVM shared library can also happen for clang-cpp, although it is less common: different tools require different versions of the library, and through transitive dependencies a process ends up with multiple copies of clang-cpp. This causes havoc with ELF, because calls meant to go one version of the library end up with another.
I've also considered introducing a symbol version globally, but for example the clang (C) library and other targets outside of LLVM/Clang, e.g. libc++, would not want that. So it's probably best if we keep it to those libraries.